Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Admin updates can trigger new backorders until the order cycle is closed #13065

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mkllnk
Copy link
Member

@mkllnk mkllnk commented Jan 9, 2025

What? Why?

We had a case when an order was updated after the order cycle closed. The backorder amendment failed because the backorder was completed already. We now handle this gracefully by skipping the amendment. But if the order cycle is still open or will open then we can actually create a new backorder. We can assume that the order cycle hasn't had a backorder yet.

What should we test?

  • Have an upcoming or open order cycle with FDC products.
  • Place an order as admin for that order cycle with FDC products.
  • A backorder in Shopify should be created.
  • After the order cycle closed, update one of the orders in OFN.
  • No new backorder should be created.

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

mkllnk added 3 commits January 9, 2025 12:10
Admins may want to pre-process some orders manually for going public.
And it's good to reserve stock for these.

At some point in the future, the supplier may have an order cycle with
its own times but the current FDC implementation allows orders at any
time.
@mkllnk mkllnk added the user facing changes Thes pull requests affect the user experience label Jan 9, 2025
@mkllnk mkllnk self-assigned this Jan 9, 2025
@mkllnk mkllnk marked this pull request as ready for review January 9, 2025 02:57
@RaggedStaff
Copy link
Collaborator

I'm away at orfc, so won't ba able to test this till next week.

I'm also a bit confused @mkllnk - I thought you said (in the issue) we couldn't create a new back order when the order cycle is closed. 😕 did you figure out a way to track the order number?

@mkllnk
Copy link
Member Author

mkllnk commented Jan 9, 2025

I'm away at orfc, so won't ba able to test this till next week.

Okay, maybe we can test it then.

I'm also a bit confused @mkllnk - I thought you said (in the issue) we couldn't create a new back order when the order cycle is closed. 😕 did you figure out a way to track the order number?

There's no technical limitation in creating backorders. And this pull request only creates backorders for order cycles before they close. After they close, we can assume that a backorder was placed and completed and then we shouldn't create a second one. But before the close time, it's okay.

@mkllnk mkllnk requested a review from dacook January 9, 2025 21:17
@mkllnk
Copy link
Member Author

mkllnk commented Jan 9, 2025

I'm away at orfc, so won't ba able to test this till next week.

Actually, next week is fine. We won't have complete code review until next week either.

Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Great fix

@@ -28,7 +28,7 @@ def amend_backorder(order)

backorder = orderer.find_open_order(order)

update(backorder, user, distributor, order_cycle)
update(backorder, user, distributor, order_cycle) if backorder
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it amounts to the same thing, but I thought that we usually avoid implied booleans and use the more explicit present?

But i'm sure it amounts to the same thing, and this looks cleaner so I like it :)


expect(backorder.lines[0].quantity).to eq 1 # beans
expect(backorder.lines[1].quantity).to eq 5 # chia
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, that's a tricky one to test. I guess the only other way would be to use VCR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
user facing changes Thes pull requests affect the user experience
Projects
Status: Code review 🔎
Development

Successfully merging this pull request may close these issues.

NoMethodError in AmendBackorderJob@default
3 participants